Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added mechanism to determine if server did not start cleanly #1539

Merged
merged 2 commits into from
Jun 4, 2023

Conversation

couling
Copy link
Contributor

@couling couling commented May 16, 2023

Same rationale as #715. I wonder if it got accidentally removed in the great 3.rewrite or if it was deliberately removed.

The key issue is starting a server and waiting for it to be up, also requires the unhappy path to be handled in determining that it will not come up.

@janiversen
Copy link
Collaborator

The patch looks good, except it breaks a bunch of tests for some reason, please fix that.

@couling
Copy link
Contributor Author

couling commented May 17, 2023

Sure I'll look into it. @janiversen i'm wondering whether it's better to set(False) or set the exception. I'm not 100% sure myself.

@janiversen
Copy link
Collaborator

Setting an exception is strange, you could raise an exception.

@couling
Copy link
Contributor Author

couling commented May 19, 2023

@janiversen I think that's what I mean. This could be written so that one or the other of these works:

if not await connection.serving:
    # Handle failed connection 
    ...
try:
    await connection.serving
except IOError:
    # Handle failed connection 
    ...

The difference in this PR is between:

except:
    self.serving.set_result(False)
    raise
except IOError:
    self.serving.set_exception(sys.exc_info()[1])
    raise

@janiversen
Copy link
Collaborator

The last can be made a lot easier:

except IOError as exc:
    self.serving.set_exception(exc)
    raise

But independent of that:

  1. you need to consider which values serving can have and how to test them "if serving" will run for both True and exc, and I suppose that is not your intention.
  2. serving needs to be documented, especially if it contains more than True/False
  3. the tests are still failing

@janiversen
Copy link
Collaborator

Cleaned up the PR.

@couling
Copy link
Contributor Author

couling commented Jun 4, 2023

Apologies for not getting back sooner. It's been a busy few weeks.

@janiversen
Copy link
Collaborator

I added a new future: serving_done.

Your concept could not work, because you cannot do set_result() on a future one, not multiple times.

@janiversen janiversen merged commit a428399 into pymodbus-dev:dev Jun 4, 2023
@couling
Copy link
Contributor Author

couling commented Jun 4, 2023

Just to check that serving_done would fit the need, the aim is to have a single code path complete on success or failure.

The problem with the existing design was that you could not "continue when server ready":

  • if you await the existing future your code hangs infinitely when the server cannot start
  • the failure is only reported to the task that otherwise serves infinitely on success

so no single code path is certain to continue on both success and failure... here continue does not need to be success, it could be continue by raising an exception but it must exit and not await indefinitely.

It's particularly problematic in unit tests, but has other uses.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants